-
-
Notifications
You must be signed in to change notification settings - Fork 78
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(skymp5-server): debug crash #2213
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ Changes requested. Reviewed everything up to 638e4a0 in 1 minute and 5 seconds
More details
- Looked at
85
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:1175
- Draft comment:
Remove the commented-outstd::terminate();
line to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%
The PR modifies theApplyChangeForm
function to log critical information instead of terminating the program. However, the commented-outstd::terminate()
line should be removed to avoid confusion.
2. skymp5-server/cpp/server_guest_lib/MpObjectReference.cpp:32
- Draft comment:
Ensure that<stacktrace>
is supported in the build environment or provide an alternative for older C++ standards. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_JRa0iFqT4pN0vLXW
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -1129,8 +1146,33 @@ MpChangeForm MpObjectReference::GetChangeForm() const | |||
void MpObjectReference::ApplyChangeForm(const MpChangeForm& changeForm) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider resetting pImpl->setPropertyTrackingInfo
after logging in ApplyChangeForm
to avoid incorrect logging in subsequent calls.
Maybe close? It won't work without either C++23 on (which needs at least a clang update) or |
@nic11 could u please send a link to a discord topic where we were discussing this. Also I want to introduce formal rules for closing PRs due to inactivity. CLosing important PRs should always lead to a ticket creation |
Important
Enhance debugging in
MpObjectReference.cpp
by tracking property changes and logging detailed information whenApplyChangeForm
is called afterSetProperty
.SetPropertyTrackingInfo
struct to track property changes inMpObjectReference.cpp
.SetProperty()
whenGetFormId()
is0xE2218
.ApplyChangeForm()
if called afterSetProperty()
.std::terminate()
with detailed logging inApplyChangeForm()
to provide more context on errors.This description was created by
for 638e4a0. It will automatically update as commits are pushed.